[codex] Preserve auth HTTP failure diagnostics#3419
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ApprovabilityVerdict: Needs human review Changes modify files in the auth directory (apps/server/src/auth/http.ts), which requires human review regardless of change complexity per security guidelines. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
0f8b837 to
edf63a7
Compare
f476f6d to
519cb0a
Compare
edf63a7 to
d430f59
Compare
519cb0a to
265d3cb
Compare
d430f59 to
fae19ef
Compare
265d3cb to
55daaa4
Compare
fae19ef to
2b5e1cf
Compare
55daaa4 to
6d9fb38
Compare
2b5e1cf to
8728ebf
Compare
6d9fb38 to
5697b83
Compare
8728ebf to
55e9cd5
Compare
5697b83 to
a1f24c7
Compare
a1f24c7 to
c26e2ff
Compare
70fdb85 to
5a80908
Compare
c26e2ff to
89040c9
Compare
553daf6 to
ed36096
Compare
89040c9 to
2d37a35
Compare
ed36096 to
15aa01a
Compare
2d37a35 to
307df64
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Interrupt missed in Cause
- Extracted the loop body into a recursive walkForInterrupt helper that, upon encountering a non-interrupt-only Cause, iterates its Fail reasons and continues walking each error's .cause chain to find nested interrupt causes.
Or push these changes by commenting:
@cursor push 5780d93e6f
Preview (5780d93e6f)
diff --git a/apps/server/src/auth/http.ts b/apps/server/src/auth/http.ts
--- a/apps/server/src/auth/http.ts
+++ b/apps/server/src/auth/http.ts
@@ -83,10 +83,27 @@
function findInterruptCause(input: unknown): Cause.Cause<never> | undefined {
const seen = new Set<object>();
+ return walkForInterrupt(input, seen, 0);
+}
+
+function walkForInterrupt(
+ input: unknown,
+ seen: Set<object>,
+ depth: number,
+): Cause.Cause<never> | undefined {
let current = input;
- for (let depth = 0; depth < MAX_CAUSE_CHAIN_DEPTH; depth += 1) {
+ for (let d = depth; d < MAX_CAUSE_CHAIN_DEPTH; d += 1) {
if (Cause.isCause(current)) {
- return Cause.hasInterruptsOnly(current) ? (current as Cause.Cause<never>) : undefined;
+ if (Cause.hasInterruptsOnly(current)) {
+ return current as Cause.Cause<never>;
+ }
+ for (const reason of current.reasons) {
+ if (Cause.isFailReason(reason)) {
+ const found = walkForInterrupt(reason.error, seen, d + 1);
+ if (found !== undefined) return found;
+ }
+ }
+ return undefined;
}
if (typeof current !== "object" || current === null || seen.has(current)) {
return undefined;You can send follow-ups to the cloud agent here.
307df64 to
07d8a37
Compare
15aa01a to
bd937be
Compare
07d8a37 to
e9e9089
Compare
bd937be to
517c1ea
Compare
517c1ea to
8d916bc
Compare
e9e9089 to
629ce09
Compare
8d916bc to
7670d78
Compare
629ce09 to
9c1fed6
Compare
7670d78 to
1ed6271
Compare
9c1fed6 to
f5fe411
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: IPC openExternal breaks boolean contract
- Added Effect.orElseSucceed(() => false) to the IPC handler so that ElectronShellOpenExternalError is caught and returns false instead of rejecting the IPC call, preserving the boolean contract for renderer callers.
Or push these changes by commenting:
@cursor push f71e08ecb0
Preview (f71e08ecb0)
diff --git a/apps/desktop/src/ipc/methods/window.ts b/apps/desktop/src/ipc/methods/window.ts
--- a/apps/desktop/src/ipc/methods/window.ts
+++ b/apps/desktop/src/ipc/methods/window.ts
@@ -141,6 +141,6 @@
result: Schema.Boolean,
handler: Effect.fn("desktop.ipc.window.openExternal")(function* (url) {
const shell = yield* ElectronShell.ElectronShell;
- return yield* shell.openExternal(url);
+ return yield* shell.openExternal(url).pipe(Effect.orElseSucceed(() => false));
}),
});You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit f5fe411. Configure here.
| cause, | ||
| }), | ||
| }).pipe(Effect.as(true)), | ||
| }); |
There was a problem hiding this comment.
IPC openExternal breaks boolean contract
Medium Severity
The ElectronShell.openExternal function now throws ElectronShellOpenExternalError when Electron rejects a URL, instead of returning false. The desktop IPC handler for openExternal still expects a boolean return, causing IPC calls to reject when the external open fails, rather than returning false as expected by renderer callers.
Reviewed by Cursor Bugbot for commit f5fe411. Configure here.
c91fad6 to
9e0c536
Compare
f5fe411 to
efacbf7
Compare
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
efacbf7 to
15a2898
Compare



Summary
catchTagshandlingValidation
vp test apps/server/src/auth/http.test.ts apps/server/src/auth/EnvironmentAuth.test.tsvp check(passes with 20 pre-existing warnings)vp run typecheckStacked on #3240.
Note
Medium Risk
Touches auth HTTP 500 and logging paths around sessions and cookies, but behavior changes are limited to diagnostics, interruption propagation, and narrower error catching—not credential validation itself.
Overview
Auth HTTP internal failures now avoid logging or encoding full
Cause/error payloads, replacing them with bounded fields (failureTag, reason/failure/defect/interrupt counts) while still attaching the original failure on a newEnvironmentHttpInternalError(public JSON stays the sameEnvironmentInternalErrorshape).failEnvironmentInternalis required to receive a cause, re-propagates nested interrupt-only failures instead of turning them into synthetic 500s, and request finalizers skip warning logs for pure interruptions. Browser session cookie handling only mapsCookieErrortobrowser_session_cookie_failedviacatchTagsinstead of a broad catch.Adds
http.test.tscoverage for redacted logging, request finalizer behavior, and interruption propagation.Reviewed by Cursor Bugbot for commit 15a2898. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Preserve auth HTTP failure diagnostics by summarizing causes instead of serializing them
failureTagand counts of failures, defects, and interruptions via a newfailureLogAttributeshelper.findInterruptCauseto detect nested interruption causes and re-propagate them directly, avoiding conversion into synthetic internal errors and suppressing redundant logs.EnvironmentHttpInternalErrorwith a boundedfailureTagfield and preserved original cause as a defect, replacing the generic internal error type.annotateEnvironmentRequestfinalizer so it skips logging entirely when the exit cause contains only interrupts.browserSessioncookie error catch to only handleCookieError, letting other errors fall through to upstream handling.Macroscope summarized 15a2898.